chore(sample): update the sample app to Angular 21 - #3671
Conversation
There was a problem hiding this comment.
new file courtesy of grok-code-fast-1, we're doing something similar in other sample apps in the project, but if there's something here we don't need, let's call that out
|
|
||
| if (isMainModule(import.meta.url)) { | ||
| // eslint-disable-next-line @typescript-eslint/dot-notation | ||
| const port = process.env['PORT'] || 4000; |
There was a problem hiding this comment.
both process.env.PORT (trying to access a property on an index type) and process.env['PORT'] (please use dot notation) have some sort of error associated with it, I went with this option
we may want to take a look at lint rules in the future
| @defer (hydrate on idle) { <app-database /> } @placeholder { Database! | ||
| … } @defer (hydrate on idle) { <app-firestore /> } @placeholder { | ||
| Firestore! … } @defer (hydrate on idle) { <app-functions /> } | ||
| @placeholder { Functions! … } @defer (hydrate on idle) { | ||
| <app-messaging /> } @placeholder { Messaging! … } @defer (hydrate on | ||
| idle) { <app-remote-config /> } @placeholder { Remote Config! … } | ||
| @defer (hydrate never) { <app-storage /> } @placeholder { Storage! … | ||
| } @defer (hydrate on idle) { <app-upboats /> } @placeholder { … } |
There was a problem hiding this comment.
I'd recommend we start using prettier for formatting (something to think about)
| "scripts": { | ||
| "ng": "ng", | ||
| "start": "firebase emulators:exec --import seed \"ng serve\"", | ||
| "start": "npx --yes firebase-tools@latest emulators:exec --import seed \"ng serve\"", |
There was a problem hiding this comment.
maybe someone had firebase-tools installed globally, but I don't and I'd prefer to follow the pattern I see in the root package.json of installing this on demand
alternatively, we could install the package in the devDependencies and keep it up to date
|
@jamesdaniels any feedback for this PR? |
|
@jamesdaniels should we try to get this in before v21 comes along? 🤓 |
Continues the Angular 19 to 20 update in this PR up to Angular 21, so the sample matches the workspace library (which is on Angular 21). - bump all @angular/* to ^21.0.0 and @angular/fire to the Angular 21 build; drop the unused, deprecated @angular/animations and the deprecated @angular/platform-browser-dynamic; bump @types/node to ^22 for the Node 22 runtime. - main.server.ts: pass the BootstrapContext that Angular 21 requires for the server bootstrap. Without it, build-time route extraction fails with NG0401. - test.ts: reconcile the test setup with the app's zoneless mode. Drop the zone.js imports, switch to @angular/platform-browser/testing, and provide zoneless change detection so fixture.detectChanges works. - rename the project ng20-test to ng21-test throughout: package.json name and serve:ssr script, angular.json project key/outputPath/buildTargets, the index.html and README titles, and AppComponent.title with its spec. Builds and unit tests pass against the Angular 21 library.
|
Thanks for this, @markgoho, and for opening #3669 to track it. I picked this up to get it into the Angular 21 release, and I pushed a few commits to your branch to carry it the rest of the way. Rather than land it at Angular 20, I took it straight to Angular 21 so the sample matches the library (the workspace is on 21 now). The sample pins On top of your 19 to 20 work, the 20 to 21 commit:
|
tyler-reitz
left a comment
There was a problem hiding this comment.
The Angular migration itself is correct and I have no objections to any of it: provideExperimentalZonelessChangeDetection → provideZonelessChangeDetection, provideServerRoutesConfig → provideServerRendering(withRoutes(...)) from @angular/ssr, the BootstrapContext threading in main.server.ts, @angular-devkit/build-angular:* → @angular/build:*, and dropping zone.js / platform-browser-dynamic / animations.
Main ask: strip the formatting churn
The sample isn't covered by the repo's lint — angular.json lints src/**/*.ts and src/**/*.html only — so none of the reformatting is required by project tooling. It looks like an editor's Prettier config, and it buries the real changes.
It also actively hurts readability in the templates. app.component.ts turns seven aligned one-line @defer blocks into eight lines of wrapped prose:
@defer (hydrate on idle) { <app-database /> } @placeholder { Database!
… } @defer (hydrate on idle) { <app-firestore /> } @placeholder {
and auth.component.ts collapses } @if (...) { onto shared lines.
If reformatting the sample is wanted, it'd be better as its own PR with a checked-in config so it doesn't drift again. Relatedly, the // eslint-disable-next-line @typescript-eslint/dot-notation added to server.ts is dead weight — nothing lints that file.
Metadata is stale
Title and description say Angular 20, but the head commit is chore(sample): carry the sample app to Angular 21 and sample/package.json now pins ^21.0.0. The author's notes (Angular 20, ~350 lint errors, --no-verify) describe an earlier state of the branch. Worth rewriting the title/body and re-verifying the checklist against what's actually here now.
Smaller points
"start": "npx --yes firebase-tools@latest emulators:exec ..."fetches an unpinned firebase-tools on every run, requires network, and can drift from the version the library peers (^14.0.0 || ^15.0.0). Prefer the workspace binary or a pinned version."@angular/fire": "file:../angular-fire-21.0.0-rc.0.tgz"now encodes an RC in the filename, so it needs a bump every release, and nothing insample/README.mdsays how to produce that tarball. Same pattern as before (19.0.0.tgz) so not a regression — but since it's being touched anyway, pointing atdist/packages-dist/would stop the churn.src/test.tsplus"main": "src/test.ts": hand-rollinginitTestEnvironmentis the pre-v20 shape.@angular/build:karmahas aprovidersFileoption meant for exactly this zoneless case — worth checking whether it works here, since it would also remove the Firebase providers now duplicated betweentest.tsandapp.component.spec.ts. And on the Vitest note: Angular 21's default for new projects is@angular/build:unit-test, so that instinct is right, just not this PR's job.app.component.spec.tschangingquerySelector('h1')?.textContent→compiled.textContentis a real fix (the template has noh1, so the old assertion was failing), but asserting'Hello World!'against the whole rendered tree is a weak check. Fine as-is — flagging so it's a deliberate choice rather than a side effect.
No behavior change. The Angular 20 migration commits carried an editor's Prettier reformatting (quotes, wrapping, import order) across every touched file, which buried the real changes. This restores main's formatting everywhere, reverts interface Animal back to the type alias, and drops a dead eslint-disable comment in server.ts (nothing lints the sample). Build and the 3 specs verified green at this commit.
… the tarball Delete the hand-rolled src/test.ts. When the test target in angular.json does not name an entry file, the @angular/build:karma builder generates the test-environment initialization itself. The Firebase test providers already lived in the spec, so the diff moves only the zoneless change-detection provider there. firebase-tools moves into devDependencies so npm start stops downloading the newest release on every run and cannot drift ahead of the firebase-tools versions the library supports. The start script runs the firebase command that install provides. The README now documents how to produce the tarball the sample installs the library from.
Angular 21 generates new applications with the @angular/build:unit-test builder running vitest, so the sample now matches what the CLI produces for its target version. The bare builder entry picks up the same defaults a new app gets. The specs run unchanged: describe, it, and expect keep working as vitest globals, so only the type declarations move from jasmine to vitest/globals. The karma and jasmine packages leave devDependencies, replaced by vitest, jsdom (the browserless DOM the tests now run in), and @vitest/coverage-v8 so ng test --coverage keeps producing reports.
The sample imports from firebase/auth directly but never declared the package, relying on the copy @angular/fire bundles being hoisted where the import can find it. An application that sets up @angular/fire through ng add carries its own firebase entry, so the sample now matches, and npm dedupes the two ranges to a single installed copy.
Angular 21 scaffolds its SSR server on express ^5.1.0, so the sample now matches, with @types/express moving to the matching major. Express 5 changed route-pattern parsing, so the catch-all handler is registered without the '/**' path string, the same shape the v21 scaffold uses. The two-step dirname/fileURLToPath computation becomes import.meta.dirname, available on every Node version Angular 21 supports. The listener block also adopts the scaffold's shape: the server now starts under the PM2 process manager too, and the listen callback throws startup errors instead of ignoring them.
The root tsconfig becomes the solution-style coordinator the v21 CLI generates: it compiles nothing itself and references the app and spec configs as separate projects, which gives editors the right typing per context. The module setting moves to preserve, which implies the bundler resolution and import interop the older explicit flags provided, and the unused outDir goes away. tsconfig.app.json switches from listing entry files by hand to including all sources except specs.
…context Calls like signInAnonymously and updateDoc were made directly inside click handlers, which run after the component's injection context is gone, so AngularFire logged its outside-injection-context warning and could not apply its change-detection and SSR safeguards there. The two affected components, auth and upboats, now capture an EnvironmentInjector and re-enter it with runInInjectionContext inside each handler, the exact pattern docs/zones.md teaches. The functions demo needs no wrap, because its callable is produced once during field initialization, inside the injection context, and the returned function never touches the injector. The raw Firebase SDK call, signInWithPopup, is outside the warning system's scope and stays as is.
The functions folder is its own small project, and without its own install and build the functions emulator skips it with a warning, so the Call! demo had no backend. The README now documents the one-time setup. Its engines entry moves from Node 20 to 22, matching the runtime the deploy schematic generates, and its TypeScript compile target moves to es2022 alongside.
The heading was the PascalCased project name the CLI scaffold derived from the folder, never a chosen title.
Nothing imports it: not the sample source and not the shipped @angular/fire code. It also sat in the build's externalDependencies list, which only matters for packages that actually appear in imports. It looks left over from an earlier approach to the session cookie handling that the current server-side code no longer uses.
angular.json drops the extract-i18n target, outputPath, and index, none of which ng new generates anymore (the removed values equal the builder's defaults), and gains the security.allowedHosts entry new apps carry. @types/node moves from ^22 to ^20.17.19, the range new apps declare. The types package should describe the OLDEST Node the app supports, which is Angular 21's 20.19 requirement, so the compiler rejects APIs that would crash there. Every API the server uses is typed well within that range.
The development-server section told readers to run bare ng serve, but every demo talks to the local emulators, so that yields a page with no backend. It now leads with npm start, which boots the seeded emulators and then serves. The scaffold's end-to-end section instructed ng e2e, which errors because no e2e target is configured, so it goes until a real e2e setup exists.
The sample app declares firebase and @angular/fire bundles its own firebase dependency. npm installs a single shared copy only while the two version ranges overlap. If they ever stop overlapping, npm installs two copies side by side and objects from one copy are rejected by the other at runtime. This assertion fails loudly the moment that happens.
|
I pushed a stack of small commits addressing everything, starting with one that reverts the reformatting, so every real change can be reviewed on its own. FormattingI restored every file whose changes were formatting-only to the version on providersFile / test.tsI went straight to your underlying instinct here, which markgoho's original notes also raised: Angular 21 makes vitest the default for new apps, so the sample now runs its tests through the bare firebase-toolsI went with your devDependency option: The tarball pathI kept the tarball but added a README section documenting how to produce it from the root build, including the note that the filename tracks the root package version. The spec assertionAgreed the check is loose, and here is why it stays that way for now. The old test looked for an Beyond the reviewWhile in here I finished carrying the sample to what Angular 21 actually generates, each as its own commit:
Everything was verified against the emulators end to end: sign-in, a Firestore write, and the Cloud Function call, with a clean console. If any of this lands differently for you, happy to adjust before it merges. |
tyler-reitz
left a comment
There was a problem hiding this comment.
Requesting changes on one thing: the install path the new README section documents doesn't work from a clean checkout. I built the library from this commit and followed it exactly:
cd sample && npm install
npm error code EINTEGRITY
npm error ... wanted sha512-obIjLH/... but got sha512-Gi7Dhmmcw56r9+2I8...
sample/package-lock.json pins the integrity hash of your tarball, and the tarball is gitignored, so everyone builds their own. Mine hashed differently from yours off the same commit.
In a minimal repro (npm 12) the other outcome is worse: when the recorded hash does match a cache entry, npm install and npm ci both install the cached tarball and ignore the newer one on disk, silently, so you get a stale library and no error.
npm install ../angular-fire-21.0.0-rc.0.tgz clears it, and ng build plus ng test pass afterwards. Documenting that as the install step is the fix I'd take. It rewrites the lock hash locally, so contributors need to leave that churn uncommitted, which is annoying but harmless.
For the record I tried file:../dist/packages-dist instead, and it's worse: npm symlinks the package, the symlink resolves firebase from the repo root's node_modules rather than the sample's, and you get two copies. Your single-copy spec fails and app.component.spec.ts dies with No Firebase App '[DEFAULT]'. Keeping the tarball is right.
Non-blocking:
<app-auth />is the one child that isn't deferred, soapp.component.spec.tsreally constructsAuthComponent(throwing in its constructor fails all three tests), which registersonIdTokenChangedagainst anAuthwith no emulator connection. Passes today, worth being deliberate.- The
__screenshots__/comment says Vitest browser mode, but the setup runs jsdom. - Please squash: the branch still carries
install deps/update angularand a merge commit. It'sBEHINDnow too.
The new single-copy spec earns its keep. I planted a second firebase copy under node_modules/@angular/fire/node_modules/ and it went red, green again once removed, and it's what caught the directory-dependency attempt above. Nothing in CI touches sample/, so that spec and your local run are the only cover any of this has.
A plain npm install fails from a fresh machine: package-lock.json records the integrity hash of the gitignored tarball, and a tarball built on another machine hashes differently (EINTEGRITY on a cold npm cache, a silently stale install on a warm one). Naming the tarball installs the file on disk. Both failure shapes and the fix were reproduced against an empty npm cache.
The bare unit-test target runs vitest in jsdom, so nothing writes __screenshots__/ and the entry's browser-mode comment was wrong.
|
Round two is in. Install pathI took your fix.
The undeferred app-authI dug into whether this is intentional rather than assuming it, and am keeping it eager:
The screenshots ignoreDropped the entry. Under this setup nothing writes that directory, the bare SquashI would rather not rewrite the branch at this point. The repo squash-merges, so it collapses to one commit when it lands either way, and I will edit the squash message at merge time so the WIP commit messages and the merge commits do not leak into If any of this lands differently for you, say the word. |
tyler-reitz
left a comment
There was a problem hiding this comment.
Both blocking items are fixed, thanks. The README now names the tarball install and both failure shapes, and the eager <app-auth /> reasoning holds up: server.ts forwards __session as authIdToken, so those listeners have to run.
On the squash: fine to leave the branch as is, but the repo has all three merge methods enabled, not just squash, so whoever merges needs to pick it deliberately or the WIP and merge commits land on main.
Worth remembering that nothing in CI touches sample/, so the single-copy spec and your emulator run are the only cover these changes have.
Issue number for this PR: #3669
Carries
sample/from Angular 19 to a proper Angular 21 app.The migration:
@angular/*21,@angular/buildbuilders, TypeScript 5.9provideExperimentalZonelessChangeDetection->provideZonelessChangeDetection,zone.jsremovedprovideServerRoutesConfig(serverRoutes)->provideServerRendering(withRoutes(serverRoutes))from@angular/ssrBootstrapContextthreading inmain.server.ts@angular/animationsand@angular/platform-browser-dynamicdroppedangular.jsoncarries the v21ng updateboilerplate (schematics naming defaults, CLI analytics off)Matching what Angular 21 generates for new apps:
@angular/build:unit-testtarget, with no hand-rolled test entry file, the test providers in the spec, and@vitest/coverage-v8keepingng test --coverageworking. The karma and jasmine packages are gonefirebasedeclared as a direct dependency, asng addproducesangular.jsontrimmed to the scaffold's shape (noextract-i18ntarget or default-valuedoutputPath/index,security.allowedHostsadded) and@types/nodeat the scaffold's^20.17rangeSample fixes along the way, each its own commit:
runInInjectionContext, thedocs/zones.mdpattern), clearing the dev-mode warningfirebase-toolsdeclared as a devDependency at^15.0.0(inside the library's supported range) and invoked from the local install instartcookie-storedependency removed, and the README retitled and pointed atnpm start(the demos need the emulators), with the deadng e2esection dropped@angular/fireresolve to the same installed firebase copy, failing loudly if the two version ranges ever stop overlappingfile:../angular-fire-21.0.0-rc.0.tgz, with tarball production documented insample/README.mdVerified locally: root build +
npm pack, sample install,ng build,ng test(4 specs, vitest), and a full emulator session exercising sign-in, a Firestore write, and the Cloud Function call with a clean console.Original author's notes
This PR builds on @markgoho's Angular 19 to 20 migration, which did the heavy lifting before the Angular 21 carry and cleanup above. His original notes, preserved: